Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not lint unnecessary_unwrap in external macros #5132

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

JohnTitor
Copy link
Member

@JohnTitor JohnTitor commented Feb 3, 2020

Fixes #5131

I think we shouldn't lint {panicking, unnecessary}_unwrap in macros, not just assert!.

changelog: Fix false positive in unnecessary_unwrap

@flip1995
Copy link
Member

flip1995 commented Feb 4, 2020

I think, we want to continue this in macros. Just not, if the is_some/... and the unwrap/... are arguments of the macro.

So

macro_rules! m {
    ($a:expr) => {
         if $a.is_some() {
              $a.unwrap();
         }
    }
}

m!(Some(42))

should still get linted.

That being said: I have no idea how differentiate these case. Maybe somehow with

/// Returns `true` if the two spans come from differing expansions (i.e., one is
/// from a macro and one isn't).
#[must_use]
pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool {
rhs.ctxt() != lhs.ctxt()
}

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 4, 2020
@JohnTitor
Copy link
Member Author

Oh, indeed! Re-covered it in 19ce66c, how about?

@flip1995
Copy link
Member

flip1995 commented Feb 4, 2020

Oh yeah, just allowing it in external macros works. At least for assert. The FP still exists when an internal macro is used though:

macro_rules! m {
    ($a:expr, $b:expr) => {
         if $a {
              $b;
         }
    }
}

let x: Option<u32> = Some(42);
m!(x.is_some(), x.unwrap())

(At least, I think so)

@JohnTitor
Copy link
Member Author

Ugh, it seems so. But I feel it's another FP and unrelated to the issue directly. If you think this PR is an improvement and doesn't have any other regressions, I'd say move on as it is and file that FP as a new issue.

@JohnTitor JohnTitor changed the title Do not lint unnecessary_unwrap in macros Do not lint unnecessary_unwrap in external macros Feb 6, 2020
@flip1995
Copy link
Member

flip1995 commented Feb 6, 2020

Yeah, lets merge this, since it fixes the most prevalent issue with assert!. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2020

📌 Commit 19ce66c has been approved by flip1995

@bors
Copy link
Contributor

bors commented Feb 6, 2020

⌛ Testing commit 19ce66c with merge 0c6d55b...

bors added a commit that referenced this pull request Feb 6, 2020
Do not lint `unnecessary_unwrap` in external macros

Fixes #5131

I think we shouldn't lint `{panicking, unnecessary}_unwrap` in macros, not just `assert!`.

changelog: Fix false positive in `unnecessary_unwrap`
@bors
Copy link
Contributor

bors commented Feb 6, 2020

💔 Test failed - checks-travis

@JohnTitor
Copy link
Member Author

Seems spurious:

thread 'integration_test' panicked at 'clone of repo failed: Error { code: -1, klass: 2, message: "failed to connect to github.com: Connection timed out" }', tests/integration.rs:22:5

@flip1995
Copy link
Member

flip1995 commented Feb 6, 2020

@bors retry

@bors
Copy link
Contributor

bors commented Feb 6, 2020

⌛ Testing commit 19ce66c with merge a3135ba...

bors added a commit that referenced this pull request Feb 6, 2020
Do not lint `unnecessary_unwrap` in external macros

Fixes #5131

I think we shouldn't lint `{panicking, unnecessary}_unwrap` in macros, not just `assert!`.

changelog: Fix false positive in `unnecessary_unwrap`
@bors
Copy link
Contributor

bors commented Feb 6, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing a3135ba to master...

@bors bors merged commit 19ce66c into rust-lang:master Feb 6, 2020
@JohnTitor JohnTitor deleted the fix-fp-in-unwrap-lint branch February 6, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible false positive: unnecessary unwrap in assert!
3 participants